-
Notifications
You must be signed in to change notification settings - Fork 234
chore(docs): 2nd gen component analysis for avatar, opacity checkerboard, swatch + swatchgroup, thumbnail #5740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(docs): 2nd gen component analysis for avatar, opacity checkerboard, swatch + swatchgroup, thumbnail #5740
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments! We'll definitely want to fill in the Resources sections here, plus review some of the callouts that the docs are making for accuracy!
152b6af
to
c5f8345
Compare
```html | ||
<!-- With link --> | ||
<a id="link" class="link" href="[href]"> | ||
<img class="image" alt="[label]" src="[src]" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I love how this showcases where the props go!
|
||
## Summary of changes | ||
|
||
### CSS => SWC implementation gaps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! There's a follow up ticket for avatar with some newly added default gradient background support. Should we call out that there's more to this component that isn't supported by CSS yet either?
Just thinking out loud- when would that work be done? Separately from the SWC migration to S2? Like a follow up once avatar has migrated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dang, that's tough. I'm tempted to call it "out of scope" for now since it wasn't in place when we first migrated the component. Do you think we're likely to miss it if we do that though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it hurts to include information about variants that we didn't implement in CSS, and I'm not sure what the exact order of implementation would be, but I would guess that we might do a single pass to bring our existing CSS over, then go back and address the other variants later, so it might not be absolutely necessary to include for the "first pass" migration.
We do have a ticket for those additional variants: CSS-1236. If that ends up happening in CSS before we migrate, we should come back and note it in these docs. But if we address it in SWC, I think we'll be ok.
I'm also hoping/assuming that we'll do some kind of design review before shipping anything so that's another fallback for missing variants to be called out!
|
||
- [CSS migration](https://github.com/adobe/spectrum-css/pull/3394) | ||
- [Spectrum 2 preview](https://spectrumcss.z13.web.core.windows.net/pr-2352/index.html?path=/docs/components-opacity-checkerboard--docs) | ||
- [React](https://react-spectrum.adobe.com/s2/index.html?path=/docs/colorslider--docs) (not a standalone component, but styles are used by the color slider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a standalone component, but styles are used by the color slider
Love this clarification, will be borrowing this for other components if it comes up.
1c7d103
to
ab5ae6a
Compare
088c63c
to
ee5fdd5
Compare
|
||
## Summary of changes | ||
|
||
### CSS => SWC implementation gaps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it hurts to include information about variants that we didn't implement in CSS, and I'm not sure what the exact order of implementation would be, but I would guess that we might do a single pass to bring our existing CSS over, then go back and address the other variants later, so it might not be absolutely necessary to include for the "first pass" migration.
We do have a ticket for those additional variants: CSS-1236. If that ends up happening in CSS before we migrate, we should come back and note it in these docs. But if we address it in SWC, I think we'll be ok.
I'm also hoping/assuming that we'll do some kind of design review before shipping anything so that's another fallback for missing variants to be called out!
- `rounding` (string) - Corner rounding: 'none', 'full' | ||
- `selected` (boolean) - Whether the swatch is selected | ||
- `shape` (string) - Shape variant: 'rectangle' | ||
- `size` (string) - Size: 'xs', 's', 'm', 'l' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it maybe because it's using a size mixin? I've noticed if the types aren't defined within the file (if they're imported) those don't get picked up either.
9e7a40d
to
ff74e36
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like most of my initial comments have been addressed, except for the ones in thumbnail. @rise-erpelding did an excellent job of documenting those via suggestions, so I won't clutter the PR with more comments on the same things!
d920c46
to
d2af5b1
Compare
169a999
into
2nd-gen-component-analysis
Description
Creates AI-generated migration documentation to analyze component differences to guide SWC migration to S2, with human vetting. The documentation serves as a bridge between the migrated Spectrum 2 CSS work and the corresponding web components, in order to help engineers understand what needs to be implemented, updated, or aligned between the two systems to guide the development of 2nd generation web components.
This batch is for the barebones components:
Motivation and context
Related issue(s)
SWC-1221
Author's checklist
I have added automated tests to cover my changes.I have included a well-written changeset if my change needs to be published.Reviewer's checklist
Includes thoughtfully written changeset if changes suggested includepatch
,minor
, ormajor
featuresAutomated tests cover all use cases and follow best practices for writingValidated on all supported browsersAll VRTs are approved before the author can update Golden HashDocumentation Quality
Cross-Reference Accuracy
metadata.json
files